Initial makefile and minishift setup by alexeykazakov · Pull Request #2 · redhat-developer/devconsole-git

alexeykazakov · 2019-03-21T01:43:24Z

Since it's unclear atm if we still want to go with API Aggregation approach I've extracted the makefile & minishift setup from redhat-developer/git-service#1 to a separate PR (this one).

xcoulon

looks good overall, just some minor comments

xcoulon · 2019-03-21T08:42:19Z

+
+
+[[constraint]]
+  branch = "master"


shouldn't we lock the constraint to a specific version here?

+1 to Xavier's comment

Good catch. We actually don't need it for now. Cleaned it up in redhat-developer/git-service@8469bca

xcoulon · 2019-03-21T08:42:29Z

+  name = "k8s.io/apiserver"
+
+[[constraint]]
+  branch = "master"


same question about the version

Done in redhat-developer/git-service@8469bca

xcoulon · 2019-03-21T08:45:20Z

+	@oc login -u system:admin
+	@echo "Creating sub resources..."
+	@echo "Creating CRDs..."
+	@oc create -f https://raw.githubusercontent.com/redhat-developer/devopsconsole-operator/master/deploy/crds/devopsconsole_v1alpha1_gitsource_crd.yaml


or we could include the redhat-developer/devopsconsole-operator/deploy/crds package as a required dependency (using dep) and create the CRD from the file in the vendor directory. It could bring more stability compared to always applying the latest version from master branch on GitHub.

I like the idea. But it looks like we can't require a dependency with no go code.
See golang/dep#1306

ah right. As a workaround, we could have a doc.go file in that package, maybe?

xcoulon · 2019-03-21T08:45:40Z

+
+.PHONY: deploy-apiserver-only
+deploy-apiserver-only:
+	@echo "Creating API Server..."


this does not do much, does it?

Correct. Just a placeholder for now.

no worries, that's what I thought ;)

xcoulon · 2019-03-21T08:45:55Z

+
+.PHONY: create-cr
+create-cr:
+	@echo "Creating Custom Resource..."


what else is it supposed to do?

It was a placeholder but we probably won't need it anyway. Removed it in redhat-developer/git-service@c31c486

xcoulon · 2019-03-21T08:49:18Z

+
+# Generate HTML representation (and show in browser) of coverage profile (based on unit tests).
+# Re-runs unit tests if coverage information is outdated.
+.PHONY: coverage-unit-html


do we need the HTML output? (I've never used it, personally.)

Removed in redhat-developer/git-service@f75c256

xcoulon · 2019-03-21T08:49:52Z

+
+[oc 3.11.0](https://docs.okd.io/latest/cli_reference/get_started_cli.html#installing-the-cli)
+
+[KVM Hypervisor](https://www.linux-kvm.org/page/Downloads)


only for Linux

Added a comment in redhat-developer/git-service@7e27a92

xcoulon · 2019-03-21T08:50:36Z

+minishift version
+```
+
+#### Install oc


not needed thanks to eval $(minishift oc-env) for those you use Minishift

Changed in redhat-developer/git-service@7e27a92

xcoulon · 2019-03-21T08:52:27Z

+
+After running this, you can verify all containers running container inside minishift using `docker ps`.
+
+**Note**: If you miss this, docker daemon inside minishift couldn't find latest built image which causes `ImagePullBackOff` as we are using `ImagePullPolicy: IfNotPresent`


couldn't find latest built image -> will not find the latest image, causing an ImagePullBackOff error ...

Fixed in redhat-developer/git-service@7e27a92

xcoulon · 2019-03-21T08:53:21Z

+func main() {
+	// Placeholder
+	for {
+		fmt.Println("Busy doing something super important...")


definitely! 😂

sbryzak

Looks good to me, just a slight concern about versioning for dependencies.

sbryzak · 2019-03-21T23:34:33Z

+
+
+[[constraint]]
+  branch = "master"


+1 to Xavier's comment

alexeykazakov · 2019-03-22T00:44:06Z

Thanks for the review @xcoulon and @sbryzak! I've addressed the review comments and merging now. If anyone has more comments we can address them in separate PRs.

Initial makefile and minishift setup

347a447

alexeykazakov requested review from MatousJobanek, dipak-pawar, nurali-techie, sbryzak and xcoulon March 21, 2019 01:44

xcoulon reviewed Mar 21, 2019

View reviewed changes

alexeykazakov added 5 commits March 21, 2019 16:46

Cleanup gopkg

8469bca

Remove custom resouce creation

c31c486

Rename apiserver to gitserver

26b2bf8

Remove gocov html generation

f75c256

readme

7e27a92

alexeykazakov requested a review from xcoulon March 22, 2019 00:36

sbryzak approved these changes Mar 22, 2019

View reviewed changes

Comment thread Gopkg.toml Outdated

[[constraint]]

branch = "master"

Copy link
Copy Markdown

sbryzak Mar 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to Xavier's comment

alexeykazakov merged commit 4588d17 into redhat-developer:master Mar 22, 2019

alexeykazakov deleted the make branch March 22, 2019 00:44


		[oc 3.11.0](https://docs.okd.io/latest/cli_reference/get_started_cli.html#installing-the-cli)

		[KVM Hypervisor](https://www.linux-kvm.org/page/Downloads)


		After running this, you can verify all containers running container inside minishift using `docker ps`.

		Note: If you miss this, docker daemon inside minishift couldn't find latest built image which causes `ImagePullBackOff` as we are using `ImagePullPolicy: IfNotPresent`

Conversation

alexeykazakov commented Mar 21, 2019

Uh oh!

xcoulon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbryzak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexeykazakov commented Mar 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants